Skip to content

feat(tree-view): implant package + migrate editor off @headless-tree#719

Open
softmarshmallow wants to merge 9 commits into
mainfrom
chore/implant-grida-tree-view
Open

feat(tree-view): implant package + migrate editor off @headless-tree#719
softmarshmallow wants to merge 9 commits into
mainfrom
chore/implant-grida-tree-view

Conversation

@softmarshmallow
Copy link
Copy Markdown
Member

@softmarshmallow softmarshmallow commented May 17, 2026

image

Summary

Adds @grida/tree-view — a headless, framework-agnostic tree-view controller with zero runtime dependencies and an optional React peer binding (@grida/tree-view/react) — and migrates the entire editor off @headless-tree, which is now removed.

  • packages/grida-tree-view — core controller, read-only TreeSource, mutations-as-intents, drag/geometry/keymap/selection/rows modules, React provider. 118 vitest cases + bench suite, README.md, FEEDBACKS.md (two first-consumer triage cycles).
  • F10 — reversed-list drag: resolveDropPosition takes { desiredDepth?, reversed? } and flips before/after so the document index matches the rendered (layer-panel) order; threaded via flatten.reverseChildren. ⚠️ The 5th-param shape change is a breaking API change, made pre-publish and triage-blessed.
  • F11.1 — tolerant reveal: expandTo/reveal skip ids the source has not snapshotted yet (a just-inserted node selected a tick early) instead of throwing and taking down the panel.
  • editorEditorTreeSource adapts the canvas document into a read-only TreeSource (identity-stable snapshots, refreshed off the editor.doc subscription, since the wasm backend keeps state refs stable so effect-deps never refire). The layers panel, scenes panel, ui-editor/tree.tsx, and the starterkit slide-list all run on @grida/tree-view; @headless-tree/{core,react} and its last demo consumers are removed.
  • editor — dev playground at app/(dev)/ui/components/tree-view and the public showcase at app/(tools)/packages/@grida/tree-view.

Follows the @grida/svg precedent (implant → dogfood). FEEDBACKS.md carries both consumer cycles (feedback → maintainer triage → status actioned).

Test plan

  • pnpm turbo test --filter='./packages/*' --filter=editor — tree-view 118 passing
  • pnpm typecheck clean (editor scope, 30/30)
  • oxlint clean
  • /canvas — layers panel renders; insert/rename/lock reflect live; F10 reversed drag-reorder verified end-to-end in-browser (domMatchesReversedDoc: true, no crash)
  • /canvas/slides — slide list drag-reorder + selection + arrow-key nav (logic preserves prior editor.doc.mv semantics)
  • Showcase /packages/@grida/tree-view and dev playground /(dev)/ui/components/tree-view render

Summary by CodeRabbit

  • New Features
    • Adds a headless tree-view library with React bindings, themed showcases (Grida/Figma/VSCode/Finder/Notion), virtualization, guides overlay, and rich drag‑and‑drop (move/copy, external palette, depth snapping).
    • New demo panels and recipes: inline rename, type‑ahead, multi‑select drag, reveal/focus, persisted expansion, decorations, large/deep fixtures, and a custom data‑source panel.
  • Documentation
    • Full README and adoption feedback log added.
  • Tests
    • Extensive unit tests and benchmarks covering geometry, drag, constraints, controller, keymap, rows, and selection.

Review Change Stack

Headless, framework-agnostic tree-view controller with zero runtime
dependencies and an optional React peer binding.

- packages/grida-tree-view: controller, source, drag, geometry, keymap,
  selection, rows + React provider; tests, bench, README, FEEDBACKS
- editor: dev playground (app/(dev)/ui/components/tree-view) and the
  public package showcase (app/(tools)/packages/@grida/tree-view)
- editor: dogfood @grida/tree-view in starterkit slide-list as the
  first production consumer (replaces @headless-tree there)
@vercel
Copy link
Copy Markdown

vercel Bot commented May 17, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
grida Ready Ready Preview, Comment May 18, 2026 7:06pm
6 Skipped Deployments
Project Deployment Actions Updated (UTC)
code Ignored Ignored May 18, 2026 7:06pm
docs Ignored Ignored Preview May 18, 2026 7:06pm
legacy Ignored Ignored May 18, 2026 7:06pm
backgrounds Skipped Skipped May 18, 2026 7:06pm
blog Skipped Skipped May 18, 2026 7:06pm
viewer Skipped Skipped May 18, 2026 7:06pm

Request Review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 17, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Introduces @grida/tree-view (core types, TreeSource, controller, drag/constraints, keymap, geometry), React bindings, comprehensive tests/benchmarks, documentation and package metadata; adds multiple demo panels, fixtures, and integrates the new library into editor consumers (slide list, scenes, and hierarchy), replacing previous headless-tree wiring.

Changes

Tree-View Library Implementation

Layer / File(s) Summary
Core types and TreeSource implementation
packages/grida-tree-view/src/types.ts, packages/grida-tree-view/src/source.ts
Public type surface and TreeSource interface with InMemoryTreeSource, traversal helpers, mutation APIs, and utility functions.
Selection adapter and row flattening
packages/grida-tree-view/src/selection.ts, packages/grida-tree-view/src/rows.ts
Pluggable SelectionAdapter with InMemorySelectionAdapter, pure selection helpers, and flattenForRender plus RowsSnapshot memoization for visible row lists.
Drag/drop, constraints and geometry
packages/grida-tree-view/src/drag.ts, packages/grida-tree-view/src/constraints.ts, packages/grida-tree-view/src/geometry.ts
Core drag primitives, resolveDropPosition and createDrag, constraint combinators (allOf, intoNearestAncestor, onlyIntoContainers, etc.), and pure geometry helpers for hit-testing and auto-scroll.
Keymap
packages/grida-tree-view/src/keymap.ts
Keymap helpers, defaultKeymap, keyComboOf and lookupAction, and selection-mode mapping.
TreeController coordination
packages/grida-tree-view/src/controller.ts
Controller owning transient UI state (expanded, focused, selection, drag), exposes subscription channels, view accessors, expansion/selection/focus/drag APIs, keyboard dispatch, and intent emission.
React bindings
packages/grida-tree-view/src/react/provider.tsx, packages/grida-tree-view/react.tsx
TreeProvider, useTree, and useTreeSnapshot for selector-based snapshots and server-safe subscriptions; React barrel export.
Public entrypoints and packaging
packages/grida-tree-view/index.ts, packages/grida-tree-view/package.json, packages/grida-tree-view/tsdown.config.ts, packages/grida-tree-view/tsconfig.json, packages/grida-tree-view/vitest.config.ts
Barrel export, package exports (., ./react), build/test config, and publishing metadata.
Docs, feedback & tooling
packages/grida-tree-view/README.md, packages/grida-tree-view/FEEDBACKS.md
Comprehensive README, feedback log, and contributor/testing doctrine.
Tests & Benchmarks
packages/grida-tree-view/__tests__/*, packages/grida-tree-view/__bench__/tree.bench.ts
Extensive unit tests covering constraints, controller, drag, geometry, keymap, rows, selection, subtree membership, intent behavior, and benchmarks for flattening and drag resolution.

Demo UI and Consumer Integration

Layer / File(s) Summary
Fixtures and JsonSource example
editor/app/(dev)/ui/components/tree-view/_fixtures.ts, editor/app/(dev)/ui/components/tree-view/_custom-source.tsx
Multiple demo fixture builders, CONTAINER_KINDS, isContainerMeta, buildLargeFixture/buildDeepFixture, and a JSON-backed JsonSource adapter example.
Demo panel and controller wiring
editor/app/(dev)/ui/components/tree-view/_panel.tsx, editor/app/(dev)/ui/components/tree-view/_virtual-panel.tsx
DemoPanel implementing hit-testing, drag orchestration, row mapping, optional virtualization via VirtualPanel, useDemoController and useRowSnapshot helpers.
Row rendering and visual guides
editor/app/(dev)/ui/components/tree-view/_row.tsx, editor/app/(dev)/ui/components/tree-view/_guides.tsx
DemoRow with selection/focus/drag/drop indicators, DropLine, and TreeGuides SVG overlay for guide rails.
Themed panels and row state
editor/app/(dev)/ui/components/tree-view/_themes.tsx, editor/app/(dev)/ui/components/tree-view/_showcase.tsx
Five themed panels (Grida, Figma, VSCode, Finder, Notion), shared hover context/useHover, grouping highlight selectors, row-state resolver, and integrated showcase sections with SVG canvas.
Recipe panels
editor/app/(dev)/ui/components/tree-view/_recipes.tsx
Eight consumer wiring recipes: inline rename, multi-select drag summary, focus-after-delete, type-ahead, reveal, external drag, decorations, persisted expanded state.
Demo page and landing
editor/app/(dev)/ui/components/tree-view/page.tsx, editor/app/(tools)/(packages)/packages/[@grida]/tree-view/page.tsx, editor/app/(tools)/(packages)/packages/[@grida]/tree-view/layout.tsx
Full demo page (TreeViewDemoPage) with many sections and a landing page + package layout for the tree-view package.
Consumer migrations
editor/grida-canvas-react-starter-kit/starterkit-slides/slide-list.tsx, editor/grida-canvas-react-starter-kit/starterkit-hierarchy/*, editor/components/ui-editor/tree.tsx
Migrates slide list and hierarchy components to use @grida/tree-view via SlideTreeSource/EditorTreeSource, updates scene/hierarchy list implementations, refactors presentational Tree primitives and NodeHierarchy components, and adapts editor wiring for controller intents.
Workspace and site updates
editor/package.json, editor/app/(dev)/ui/page.tsx, editor/app/(tools)/(packages)/packages/data.ts, editor/app/(tools)/(packages)/packages/sitemap.ts
Adds workspace dependency, updates components index entry, package list metadata and sitemap entry.

Estimated code review effort: 🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs:

  • gridaco/grida#349: Prior migration effort replacing headless-tree wiring; shares context with this migration.

Suggested labels: enhancement, canvas, ux

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/implant-grida-tree-view

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c5d4a003b2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +192 to +194
for (const a of ancestorsOf(this.source, id)) {
if (a === this.source.getRoot()) continue;
if (!this._expanded.has(a)) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Expand root when revealing rows in showRoot mode

Do not unconditionally skip the root in expandTo: when a source enables showRoot(), the root starts collapsed unless explicitly expanded, so reveal(id) expands intermediate ancestors but leaves the root collapsed and the target row still invisible. This breaks reveal/focus flows for consumers that render the root row (e.g. file-system style trees) because the requested node cannot become visible.

Useful? React with 👍 / 👎.

Comment thread packages/grida-tree-view/src/keymap.ts Outdated
const combo = keyComboOf(event);
const direct = keymap[combo];
if (direct) return direct;
if (combo !== event.key) return keymap[event.key];
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Normalize bare-key fallback in keymap lookup

Use the normalized key for the fallback lookup instead of event.key directly. keyComboOf normalizes space to "Space", but the fallback path uses raw event.key, so modified combos like Ctrl+Shift+Space miss the documented “fall back to bare key” behavior and fail to resolve Space bindings.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 18

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@editor/app/`(dev)/ui/components/tree-view/_custom-source.tsx:
- Around line 81-83: getRoot currently returns the hardcoded string "doc", which
breaks adapters created with non-"doc" roots; change getRoot() in the class that
extends JsonSource to return the instance's configured root (the
constructor-provided root property—e.g., use the class field that stores the
JsonSource root such as this.root or this.source.root) instead of the literal
"doc", optionally falling back to "doc" only if the instance root is undefined.

In `@editor/app/`(dev)/ui/components/tree-view/_fixtures.ts:
- Around line 775-784: The fixture's total count is underreporting because each
group contributes depth chain nodes plus one group row; update the returned
total in the fixture (the object that sets source: new InMemoryTreeSource(...),
expanded, maxDepth: depth) to account for the group row (for example compute
total as groups * (depth + 1) or otherwise sum group rows plus chain nodes) so
the demo reports the correct visible row count.
- Around line 722-752: The buildDeepFixture function currently builds chainIds
with for (let d = 0; d < depth; d++) which allows depth <= 0 to produce an empty
chain and then uses chainIds[chainIds.length - 1]! causing undefined node ids;
add an input guard at the start of buildDeepFixture to validate and coerce/throw
for invalid opts.depth and opts.groups (e.g., ensure depth >= 1 and groups >= 1)
or default them to safe minimums, and then proceed to build chainIds and push
nodes (references: buildDeepFixture, chainIds, depth, groups, nodes.push) so no
empty arrays are accessed at chainIds[chainIds.length - 1].

In `@editor/app/`(dev)/ui/components/tree-view/_panel.tsx:
- Around line 291-339: The drag handlers currently process any pointer's events
once a drag has started; restrict updates and commit to the initiating pointer
only by checking pointerId. In onMove (function onMove) after determining the
active drag handle (dragRef.current) compare e.pointerId to the initiating
pointer id (store the initiating id when creating the drag from
pendingRef.current.pointerId or set a dedicated activePointerId on
dragRef/current state) and return if they differ before updating lastPointerRef,
calling handle.over, or kicking auto-scroll. Similarly change onUp to accept the
PointerEvent and only call controller.commitDrag() / clear
dragRef.pendingRef.lastPointerRef when e.pointerId matches the initiating
pointer id; ensure you clear the stored initiating pointer id when the drag
ends.

In `@editor/app/`(dev)/ui/components/tree-view/_recipes.tsx:
- Around line 184-192: The onKeyDown handler calls onCancel() when Escape but
the onBlur handler still calls onCommit(value.trim() || initial), so Escape can
be followed by a blur commit; fix by introducing a short-lived flag (e.g.,
ignoreBlurRef or canceledRef) used by the onKeyDown Escape branch to set true
and by onBlur to skip calling onCommit if the flag is set, then reset the flag
(false) after ignoring the blur; update the handlers around onKeyDown, onBlur,
onCommit, onCancel and the value/initial logic accordingly.
- Around line 446-512: The effect currently depends on overInfo causing
listeners to be removed/re-added on every hover update and onUp reads a stale
closure value; fix by making the handlers stable and reading live state via
refs: remove overInfo from the effect dependency list, create an overInfoRef
that you update whenever you call setOverInfo, and inside the effect define
onMove and onUp to read/write overInfoRef.current (and use
dragRef/containerRef/controller as needed) so the global pointermove/pointerup
listeners are added once and onUp uses the latest hit value; keep controller in
deps only if it can change or otherwise capture it via a ref too.

In `@editor/app/`(dev)/ui/components/tree-view/_row.tsx:
- Around line 129-132: The left-pointer onPointerDown handler (the onPointerDown
that calls onDragStart?.(row.id, e)) is firing even when clicking the chevron;
add a guard that detects if the event target is inside the chevron element and
return early so we don't arm drag. Concretely: mark the chevron DOM node with a
stable identifier (e.g. data-chevron or class "chevron") and update the
onPointerDown (and the similar pointer handlers around lines 163-169) to check
if (e.target instanceof HTMLElement && e.target.closest('[data-chevron],
.chevron')) return; before calling onDragStart/onPointer handlers; keep using
row.id and onDragStart so the rest of behavior is unchanged.

In `@editor/app/`(dev)/ui/components/tree-view/_themes.tsx:
- Around line 97-104: selectionSubtree can throw when controller.getSelection()
contains node ids that were removed because source.getNode(id) may return
undefined or throw; update selectionSubtree to first guard/filter selection ids
against the source (e.g., use a safe check like source.hasNode(id) or check that
source.getNode(id) exists before accessing .meta) before calling isContainerKind
and building containerAnchors, and ensure any access to source.getNode(id).meta
is null-checked to avoid crashes during render in the selectionSubtree function
and related uses of sel/containerAnchors.

In `@editor/app/`(tools)/(packages)/packages/[%40grida]/tree-view/page.tsx:
- Around line 64-75: The two Link components rendering external links (the ones
using NPM_URL and GITHUB_URL in page.tsx that include target="_blank") should
add rel="noopener noreferrer" to avoid exposing window.opener; update the Link
props for the NPM and GitHub links (the Link elements wrapping the
Button/NpmLogoIcon and Button/GithubIcon) to include rel="noopener noreferrer"
alongside target="_blank".

In `@packages/grida-tree-view/__tests__/_helpers.ts`:
- Around line 39-45: The buildLinear helper currently always pushes a root node
with children ["n0"] which leaves a dangling reference when depth === 0; update
buildLinear to handle depth=0 by making the root's children an empty array in
that case (i.e., conditionally set the root node's children to [] when depth ===
0), and ensure the loop that pushes nodes for `n${i}` still only runs when depth
> 0 so no `n0` is referenced without being created.

In `@packages/grida-tree-view/__tests__/into-nearest-ancestor.test.ts`:
- Around line 5-8: The helper isFolder has been passed to intoNearestAncestor
with unsafe "as never" casts that disable type checking; replace those casts by
giving isFolder the exact predicate signature expected by intoNearestAncestor
(match parameter types and return boolean) or create a properly typed wrapper
that adapts it—e.g., ensure the function signature uses the same TreeSource and
NodeId types (or a constrained generic like (source: TreeSource<any>, id:
NodeId) => boolean) so calls to intoNearestAncestor use isFolder without any "as
never" assertion and retain full type safety.

In `@packages/grida-tree-view/README.md`:
- Line 189: Fix the wording typo in the README table row for "Selection" /
"SelectionAdapter": change the phrase "standalone uses get a default" to
"standalone users get a default" so the row reads "Editors with existing
selection plug in; standalone users get a default." Locate the table entry
mentioning Selection and SelectionAdapter and update that cell text accordingly.
- Line 25: Update the inconsistent test-count phrasing in the README by choosing
a single, accurate statement (either a specific number or a non-numeric phrase
like "≈90 unit tests" or "dozens of unit tests") and apply it to both
occurrences: the sentence beginning "Tested at the unit level. ~90
Node-environment tests..." and the later lines that currently state "~80 tests";
ensure both phrases match exactly and reflect the current test suite.

In `@packages/grida-tree-view/src/constraints.ts`:
- Around line 50-57: The not function returns a new MoveConstraint but currently
drops c.resolveDropPosition contrary to its docstring; update not (the exported
function not that takes MoveConstraint c) to return an object that keeps the
negated canMove and also forwards resolveDropPosition (e.g. include
resolveDropPosition: c.resolveDropPosition if present) so wrapped constraints
that rely on coercion still work; ensure you reference c.resolveDropPosition and
c.canMove when constructing the returned MoveConstraint.

In `@packages/grida-tree-view/src/controller.ts`:
- Around line 88-95: When the underlying source emits changes (see the
subscription callbacks created in the blocks containing this._unsubscribeSource
= this.source.subscribe(...) and the other occurrences around the ranges noted),
stale IDs can remain in _focused and in the selection adapter so later calls to
source.getNode(...) throw; update those subscription handlers to normalize state
after invalidating rows: iterate over _focused and the selectionAdapter's ids
and remove or clear any id for which source.getNode(id) (or a source.has/id
existence check) returns falsy, then emit the appropriate events (e.g., "rows"
and "selection") so the UI sees the cleaned state. Ensure this logic is added to
the three subscription locations referenced (around the existing calls to
this._rows.invalidate(), this.emit("rows"), and the other source subscription
blocks).

In `@packages/grida-tree-view/src/drag.ts`:
- Around line 72-80: resolveDropPosition currently treats placement === "into"
as valid for any target row which can produce a parent that isn't a container;
update resolveDropPosition to verify the over node can accept children before
returning the "into" result: check the overNode (e.g., overNode.isContainer or
another appropriate property/method that indicates it can have children) and
only allow the "into" branch when that check passes, otherwise fall back to a
different placement (e.g., "after"/"before") or return null/invalid. Ensure you
reference and use the existing symbols over, overNode, placement, and itemSet in
the function so the filtered children logic stays intact but only runs when the
overNode is a valid container.

In `@packages/grida-tree-view/src/keymap.ts`:
- Around line 95-99: The bare-key fallback in lookupAction incorrectly uses
event.key directly and bypasses normalizeKey; change the fallback to
normalizeKey(event.key) (or call keyComboOf with only the key) so that bare-key
bindings like space (" " vs "Space") are normalized before lookup — update the
branch that currently returns keymap[event.key] to use normalizeKey(event.key)
(or keyComboOf(event) variants) and keep the initial combo check using
keyComboOf and keymap as-is.

In `@packages/grida-tree-view/src/source.ts`:
- Around line 117-125: The remove(id: NodeId) method currently only deletes the
single node but leaves its descendants in _nodes; change remove to collect the
full subtree (e.g., using a stack/queue starting from id, repeatedly pushing
each node's children from getNode(nodeId)) and then remove every collected id
from this._nodes, also update the parent's children array (as current code does)
and finally call this.bump(); ensure you handle the case when node or
node.parent is missing before mutating.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: aef27427-b198-479a-9df6-81a05a4785d2

📥 Commits

Reviewing files that changed from the base of the PR and between 5b6e8ce and c5d4a00.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (47)
  • editor/app/(dev)/ui/components/tree-view/_custom-source.tsx
  • editor/app/(dev)/ui/components/tree-view/_fixtures.ts
  • editor/app/(dev)/ui/components/tree-view/_guides.tsx
  • editor/app/(dev)/ui/components/tree-view/_panel.tsx
  • editor/app/(dev)/ui/components/tree-view/_recipes.tsx
  • editor/app/(dev)/ui/components/tree-view/_row.tsx
  • editor/app/(dev)/ui/components/tree-view/_themes.tsx
  • editor/app/(dev)/ui/components/tree-view/_virtual-panel.tsx
  • editor/app/(dev)/ui/components/tree-view/page.tsx
  • editor/app/(dev)/ui/page.tsx
  • editor/app/(tools)/(packages)/packages/[%40grida]/tree-view/layout.tsx
  • editor/app/(tools)/(packages)/packages/[%40grida]/tree-view/page.tsx
  • editor/app/(tools)/(packages)/packages/data.ts
  • editor/app/(tools)/(packages)/packages/sitemap.ts
  • editor/grida-canvas-react-starter-kit/starterkit-slides/slide-list.tsx
  • editor/package.json
  • packages/grida-tree-view/FEEDBACKS.md
  • packages/grida-tree-view/README.md
  • packages/grida-tree-view/__bench__/tree.bench.ts
  • packages/grida-tree-view/__tests__/_helpers.ts
  • packages/grida-tree-view/__tests__/constraints.test.ts
  • packages/grida-tree-view/__tests__/controller.test.ts
  • packages/grida-tree-view/__tests__/drag.test.ts
  • packages/grida-tree-view/__tests__/geometry.test.ts
  • packages/grida-tree-view/__tests__/intent.test.ts
  • packages/grida-tree-view/__tests__/into-nearest-ancestor.test.ts
  • packages/grida-tree-view/__tests__/keymap.test.ts
  • packages/grida-tree-view/__tests__/next-focus.test.ts
  • packages/grida-tree-view/__tests__/rows.test.ts
  • packages/grida-tree-view/__tests__/selection.test.ts
  • packages/grida-tree-view/__tests__/subtree.test.ts
  • packages/grida-tree-view/index.ts
  • packages/grida-tree-view/package.json
  • packages/grida-tree-view/react.tsx
  • packages/grida-tree-view/src/constraints.ts
  • packages/grida-tree-view/src/controller.ts
  • packages/grida-tree-view/src/drag.ts
  • packages/grida-tree-view/src/geometry.ts
  • packages/grida-tree-view/src/keymap.ts
  • packages/grida-tree-view/src/react/provider.tsx
  • packages/grida-tree-view/src/rows.ts
  • packages/grida-tree-view/src/selection.ts
  • packages/grida-tree-view/src/source.ts
  • packages/grida-tree-view/src/types.ts
  • packages/grida-tree-view/tsconfig.json
  • packages/grida-tree-view/tsdown.config.ts
  • packages/grida-tree-view/vitest.config.ts

Comment thread editor/app/(dev)/ui/components/tree-view/_custom-source.tsx
Comment on lines +722 to +752
export function buildDeepFixture(opts: { groups: number; depth: number }): {
source: InMemoryTreeSource<{ label: string }>;
expanded: NodeId[];
maxDepth: number;
total: number;
} {
const { groups, depth } = opts;
const nodes: TreeNode<{ label: string }>[] = [];
nodes.push({
id: "<root>",
parent: null,
children: [],
meta: { label: "Document" },
});
const rootChildren: NodeId[] = [];
const expanded: NodeId[] = [];
for (let g = 0; g < groups; g++) {
const groupId = `g${g}`;
rootChildren.push(groupId);
expanded.push(groupId);
// Build the chain bottom-up so every parent's children array is final
// by the time we push it.
const chainIds: string[] = [];
for (let d = 0; d < depth; d++) chainIds.push(`g${g}-n${d}`);
// Last node has no children.
nodes.push({
id: chainIds[chainIds.length - 1]!,
parent: chainIds[chainIds.length - 2] ?? groupId,
children: [],
meta: { label: `g${g} · depth ${depth - 1}` },
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard depth/groups inputs before building chain nodes.

depth <= 0 makes chainIds empty, so chainIds[chainIds.length - 1]! becomes undefined and creates invalid nodes at runtime.

Suggested fix
 export function buildDeepFixture(opts: { groups: number; depth: number }): {
   source: InMemoryTreeSource<{ label: string }>;
   expanded: NodeId[];
   maxDepth: number;
   total: number;
 } {
   const { groups, depth } = opts;
+  if (groups <= 0 || depth <= 0) {
+    throw new Error("buildDeepFixture: `groups` and `depth` must be > 0");
+  }
   const nodes: TreeNode<{ label: string }>[] = [];
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function buildDeepFixture(opts: { groups: number; depth: number }): {
source: InMemoryTreeSource<{ label: string }>;
expanded: NodeId[];
maxDepth: number;
total: number;
} {
const { groups, depth } = opts;
const nodes: TreeNode<{ label: string }>[] = [];
nodes.push({
id: "<root>",
parent: null,
children: [],
meta: { label: "Document" },
});
const rootChildren: NodeId[] = [];
const expanded: NodeId[] = [];
for (let g = 0; g < groups; g++) {
const groupId = `g${g}`;
rootChildren.push(groupId);
expanded.push(groupId);
// Build the chain bottom-up so every parent's children array is final
// by the time we push it.
const chainIds: string[] = [];
for (let d = 0; d < depth; d++) chainIds.push(`g${g}-n${d}`);
// Last node has no children.
nodes.push({
id: chainIds[chainIds.length - 1]!,
parent: chainIds[chainIds.length - 2] ?? groupId,
children: [],
meta: { label: `g${g} · depth ${depth - 1}` },
});
export function buildDeepFixture(opts: { groups: number; depth: number }): {
source: InMemoryTreeSource<{ label: string }>;
expanded: NodeId[];
maxDepth: number;
total: number;
} {
const { groups, depth } = opts;
if (groups <= 0 || depth <= 0) {
throw new Error("buildDeepFixture: `groups` and `depth` must be > 0");
}
const nodes: TreeNode<{ label: string }>[] = [];
nodes.push({
id: "<root>",
parent: null,
children: [],
meta: { label: "Document" },
});
const rootChildren: NodeId[] = [];
const expanded: NodeId[] = [];
for (let g = 0; g < groups; g++) {
const groupId = `g${g}`;
rootChildren.push(groupId);
expanded.push(groupId);
// Build the chain bottom-up so every parent's children array is final
// by the time we push it.
const chainIds: string[] = [];
for (let d = 0; d < depth; d++) chainIds.push(`g${g}-n${d}`);
// Last node has no children.
nodes.push({
id: chainIds[chainIds.length - 1]!,
parent: chainIds[chainIds.length - 2] ?? groupId,
children: [],
meta: { label: `g${g} · depth ${depth - 1}` },
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/app/`(dev)/ui/components/tree-view/_fixtures.ts around lines 722 -
752, The buildDeepFixture function currently builds chainIds with for (let d =
0; d < depth; d++) which allows depth <= 0 to produce an empty chain and then
uses chainIds[chainIds.length - 1]! causing undefined node ids; add an input
guard at the start of buildDeepFixture to validate and coerce/throw for invalid
opts.depth and opts.groups (e.g., ensure depth >= 1 and groups >= 1) or default
them to safe minimums, and then proceed to build chainIds and push nodes
(references: buildDeepFixture, chainIds, depth, groups, nodes.push) so no empty
arrays are accessed at chainIds[chainIds.length - 1].

Comment thread editor/app/(dev)/ui/components/tree-view/_fixtures.ts
Comment thread editor/app/(dev)/ui/components/tree-view/_panel.tsx
Comment thread editor/app/(dev)/ui/components/tree-view/_recipes.tsx
Comment on lines +50 to +57
/** Negate a constraint's `canMove`. Does not touch `resolveDropPosition`. */
export function not(c: MoveConstraint): MoveConstraint {
return {
canMove(items, to, source) {
return !c.canMove(items, to, source);
},
};
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

not() currently strips resolveDropPosition despite documented behavior.

At Line 50 the docs say resolveDropPosition is untouched, but the returned object omits it. This changes behavior for wrapped constraints that rely on coercion.

Suggested fix
 export function not(c: MoveConstraint): MoveConstraint {
   return {
     canMove(items, to, source) {
       return !c.canMove(items, to, source);
     },
+    resolveDropPosition: c.resolveDropPosition,
   };
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/grida-tree-view/src/constraints.ts` around lines 50 - 57, The not
function returns a new MoveConstraint but currently drops c.resolveDropPosition
contrary to its docstring; update not (the exported function not that takes
MoveConstraint c) to return an object that keeps the negated canMove and also
forwards resolveDropPosition (e.g. include resolveDropPosition:
c.resolveDropPosition if present) so wrapped constraints that rely on coercion
still work; ensure you reference c.resolveDropPosition and c.canMove when
constructing the returned MoveConstraint.

Comment on lines +88 to +95
this._unsubscribeSource = this.source.subscribe(() => {
this._rows.invalidate();
this.emit("rows");
});
this._unsubscribeSelection = this.selectionAdapter.subscribe(() => {
this.emit("selection");
});
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Normalize focused/selected ids when the source changes.

After mutations, _focused (and selection entries) can point to removed ids; subsequent keyboard actions can throw via source.getNode(...).

Suggested direction
     this._unsubscribeSource = this.source.subscribe(() => {
+      // Drop invalid focus
+      if (this._focused) {
+        try {
+          this.source.getNode(this._focused);
+        } catch {
+          this._focused = null;
+          this.emit("focus");
+        }
+      }
+      // Prune invalid selection ids
+      const sel = this.selectionAdapter.get();
+      const nextSel = sel.filter((id) => {
+        try {
+          this.source.getNode(id);
+          return true;
+        } catch {
+          return false;
+        }
+      });
+      if (nextSel.length !== sel.length) {
+        this.selectionAdapter.set(nextSel, "replace");
+      }
       this._rows.invalidate();
       this.emit("rows");
     });

Also applies to: 306-314, 487-490

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/grida-tree-view/src/controller.ts` around lines 88 - 95, When the
underlying source emits changes (see the subscription callbacks created in the
blocks containing this._unsubscribeSource = this.source.subscribe(...) and the
other occurrences around the ranges noted), stale IDs can remain in _focused and
in the selection adapter so later calls to source.getNode(...) throw; update
those subscription handlers to normalize state after invalidating rows: iterate
over _focused and the selectionAdapter's ids and remove or clear any id for
which source.getNode(id) (or a source.has/id existence check) returns falsy,
then emit the appropriate events (e.g., "rows" and "selection") so the UI sees
the cleaned state. Ensure this logic is added to the three subscription
locations referenced (around the existing calls to this._rows.invalidate(),
this.emit("rows"), and the other source subscription blocks).

Comment thread packages/grida-tree-view/src/drag.ts Outdated
Comment on lines +72 to +80
if (placement === "into") {
const filtered = overNode.children.filter((c) => !itemSet.has(c));
return {
parent: over,
index: filtered.length,
placement: "into",
over,
};
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Block "into" drops on non-container targets.

resolveDropPosition currently allows placement === "into" for any row. That can produce a drop parent that is not a valid container, yielding invalid move intents downstream.

Suggested fix
   if (placement === "into") {
+    if (!source.isContainer(over)) return null;
     const filtered = overNode.children.filter((c) => !itemSet.has(c));
     return {
       parent: over,
       index: filtered.length,
       placement: "into",
       over,
     };
   }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/grida-tree-view/src/drag.ts` around lines 72 - 80,
resolveDropPosition currently treats placement === "into" as valid for any
target row which can produce a parent that isn't a container; update
resolveDropPosition to verify the over node can accept children before returning
the "into" result: check the overNode (e.g., overNode.isContainer or another
appropriate property/method that indicates it can have children) and only allow
the "into" branch when that check passes, otherwise fall back to a different
placement (e.g., "after"/"before") or return null/invalid. Ensure you reference
and use the existing symbols over, overNode, placement, and itemSet in the
function so the filtered children logic stays intact but only runs when the
overNode is a valid container.

Comment thread packages/grida-tree-view/src/keymap.ts
Comment thread packages/grida-tree-view/src/source.ts
Accept the valid findings; skip the ones that fight the package doctrine.

Package:
- expandTo no longer skips the root when showRoot() is on (P1: reveal
  left deep rows invisible for file-system-style trees)
- lookupAction normalizes the bare-key fallback (Ctrl+Shift+Space etc.
  now resolve Space bindings)
- controller tolerates a focus cursor the external source already
  removed (focusParent/canExpand no longer throw) — minimal use-site
  guard, not a per-emit selection prune
- InMemoryTreeSource.remove() deletes the whole subtree, not just root
- not(): clarified doc — deliberately exposes no resolveDropPosition
  (forwarding it into a negated predicate is incoherent)
- drop the `as never` casts in into-nearest-ancestor test; README
  test-count + wording fixes

Editor demos:
- JsonSource.getRoot() returns the real root id
- rename recipe: Escape no longer commits via onBlur
- ExternalDragPanel: stable listeners + ref (no per-hover rebind)
- panel drag: ignore non-initiating pointers (multi-touch)
- row: chevron click no longer arms drag; themed selector tolerates
  stale selection ids; deep-fixture row count; rel=noopener links

Skipped (doctrine): hard "into" container rule in the raw resolver
(constraint-driven by design), and unreachable depth<=0 guards in
internal fixtures.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/grida-tree-view/src/keymap.ts`:
- Around line 50-51: The key matching fails because single-letter keys aren't
case-normalized: update normalizeKey (used by keyComboOf) to convert
single-character alphabetical keys to uppercase so that events with
event.key="a" map to the keymap entries like "Mod+A"; specifically, detect when
the normalized key is a single ASCII letter and call .toUpperCase() (preserving
existing modifier normalization logic) so keyComboOf produces "Mod+A" and
matches the keymap entry "Mod+A".
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e5dec893-6247-4b68-8c40-0f47b4a86a9d

📥 Commits

Reviewing files that changed from the base of the PR and between c5d4a00 and 0a0af01.

📒 Files selected for processing (13)
  • editor/app/(dev)/ui/components/tree-view/_custom-source.tsx
  • editor/app/(dev)/ui/components/tree-view/_fixtures.ts
  • editor/app/(dev)/ui/components/tree-view/_panel.tsx
  • editor/app/(dev)/ui/components/tree-view/_recipes.tsx
  • editor/app/(dev)/ui/components/tree-view/_row.tsx
  • editor/app/(dev)/ui/components/tree-view/_themes.tsx
  • editor/app/(tools)/(packages)/packages/[%40grida]/tree-view/page.tsx
  • packages/grida-tree-view/README.md
  • packages/grida-tree-view/__tests__/into-nearest-ancestor.test.ts
  • packages/grida-tree-view/src/constraints.ts
  • packages/grida-tree-view/src/controller.ts
  • packages/grida-tree-view/src/keymap.ts
  • packages/grida-tree-view/src/source.ts
✅ Files skipped from review due to trivial changes (1)
  • packages/grida-tree-view/README.md
🚧 Files skipped from review as they are similar to previous changes (9)
  • editor/app/(tools)/(packages)/packages/[%40grida]/tree-view/page.tsx
  • packages/grida-tree-view/src/source.ts
  • editor/app/(dev)/ui/components/tree-view/_row.tsx
  • packages/grida-tree-view/tests/into-nearest-ancestor.test.ts
  • packages/grida-tree-view/src/constraints.ts
  • packages/grida-tree-view/src/controller.ts
  • editor/app/(dev)/ui/components/tree-view/_panel.tsx
  • editor/app/(dev)/ui/components/tree-view/_recipes.tsx
  • editor/app/(dev)/ui/components/tree-view/_fixtures.ts

Comment on lines +50 to +51
"Mod+A": "select-all",
"Shift+ArrowUp": "focus-prev",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

According to MDN/spec behavior for KeyboardEvent.key, what key value is produced for Ctrl+A without Shift, and should keybinding systems normalize letter casing for stable shortcut matching?

💡 Result:

According to MDN and W3C specifications, the KeyboardEvent.key value produced for the 'A' key when pressed in combination with the Ctrl modifier (without Shift) is "a" [1]. The key property reflects the character that would be generated by the key press, taking into account the state of modifier keys like Shift, but it does not include the modifier keys themselves in the string value [1]. Regarding keybinding systems, it is generally recommended to normalize letter casing for stable shortcut matching [2][3]. Because KeyboardEvent.key is sensitive to the Shift modifier (e.g., producing "A" when Shift is held and "a" when it is not) [1], keybinding logic that does not normalize casing can lead to brittle or broken shortcuts [2][3]. Normalizing input to lowercase (or a consistent canonical form) allows systems to treat "Ctrl+A" and "Ctrl+Shift+A" as distinct, predictable actions while avoiding issues where a user's accidental Caps Lock or Shift state prevents a shortcut from triggering [2]. Many robust systems explicitly transform uppercase inputs into a combination of the lowercase character and the Shift modifier to ensure consistent behavior [2].

Citations:


🏁 Script executed:

cd packages/grida-tree-view/src && wc -l keymap.ts

Repository: gridaco/grida

Length of output: 71


🏁 Script executed:

cat -n packages/grida-tree-view/src/keymap.ts | head -120

Repository: gridaco/grida

Length of output: 3812


Normalize letter keys to uppercase for stable keymap matching.

Ctrl+A produces event.key="a" (lowercase) per W3C spec, but line 50 registers "Mod+A" (uppercase). The current normalizeKey function (lines 81–84) doesn't normalize letter casing, so keyComboOf builds "Mod+a", which fails to match the keymap entry and breaks the select-all shortcut.

The proposed fix correctly normalizes single-character keys to uppercase before keymap lookup:

Proposed fix
 function normalizeKey(key: string): string {
   if (key === " ") return "Space";
-  return key;
+  if (key.length === 1) return key.toUpperCase();
+  return key;
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/grida-tree-view/src/keymap.ts` around lines 50 - 51, The key
matching fails because single-letter keys aren't case-normalized: update
normalizeKey (used by keyComboOf) to convert single-character alphabetical keys
to uppercase so that events with event.key="a" map to the keymap entries like
"Mod+A"; specifically, detect when the normalized key is a single ASCII letter
and call .toUpperCase() (preserving existing modifier normalization logic) so
keyComboOf produces "Mod+A" and matches the keymap entry "Mod+A".

…or off @headless-tree

Core (@grida/tree-view):
- F10: resolveDropPosition takes { desiredDepth?, reversed? } and flips
  before/after under reversed lists so document index matches the
  rendered (layer-panel) order; threaded through createDrag/TreeController
  via flatten.reverseChildren. NOTE: the 5th-param shape change is a
  breaking API change, made pre-publish and triage-blessed.
- F11.1: expandTo/reveal walk via _peek and skip ids the source has not
  snapshotted yet (e.g. just-inserted, selected a tick early) instead of
  throwing and taking down the panel.

Editor wiring (drops @headless-tree entirely):
- EditorTreeSource adapts the canvas document into a read-only TreeSource
  with identity-stable snapshots; refreshed off editor.doc subscription
  (the wasm backend keeps state refs stable, so effect-deps never refire).
- Layers + scenes panels and ui-editor/tree.tsx rewired onto
  @grida/tree-view; @headless-tree/{core,react} and their last demo
  consumers removed.

FEEDBACKS.md carries the Consumer #2 cycle (feedback -> maintainer triage
-> status actioned) mirroring Consumer #1.
Update the dev showcase (_fixtures/_themes/_showcase) and the
(tools)/packages tree-view page to exercise the post-F10 API
(reversed-list drag) and replace the retired @headless-tree demo.
@vercel vercel Bot temporarily deployed to Preview – backgrounds May 18, 2026 16:56 Inactive
@vercel vercel Bot temporarily deployed to Preview – viewer May 18, 2026 16:56 Inactive
@vercel vercel Bot temporarily deployed to Preview – blog May 18, 2026 16:56 Inactive
@softmarshmallow softmarshmallow changed the title chore(packages): implant @grida/tree-view package feat(tree-view): implant package + migrate editor off @headless-tree May 18, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

♻️ Duplicate comments (1)
packages/grida-tree-view/src/drag.ts (1)

93-101: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject into drops on non-container rows.

resolveDropPosition() still returns an into destination without checking source.isContainer(over), so leaf rows can produce invalid drop targets and downstream move intents.

Suggested fix
   if (dir === "into") {
+    if (!source.isContainer(over)) return null;
     const filtered = overNode.children.filter((c) => !itemSet.has(c));
     return {
       parent: over,
       index: filtered.length,
       placement: "into",
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/grida-tree-view/src/drag.ts` around lines 93 - 101,
resolveDropPosition currently returns an "into" destination for dir === "into"
without verifying the target can contain children; update the branch in
resolveDropPosition to call source.isContainer(over) (or similar API used for
container checks) and if it returns false, reject the "into" drop (e.g., return
null or fall back to a non-"into" placement) instead of returning an invalid
into target; adjust the code around overNode, filtered, and the return object so
the into case only executes when source.isContainer(over) is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@editor/app/`(dev)/ui/components/tree-view/_showcase.tsx:
- Around line 239-249: The stage loses keyboard focus after clicking shapes
because clicking only updates tree selection; update the pick/selection handler
inside SvgStage (the code that responds to mouse picks and calls
controller.select / selection updates) to also set focus to the stage container
element (ensure the stage root has a tabbable attribute like tabIndex if not
already) and call .focus() after selecting a shape so Delete/Backspace shortcuts
bound on the stage container will work immediately; apply the same change to the
other mouse-pick handlers referenced in the same file (the other pick handlers
around lines 273–319).

In `@editor/app/`(dev)/ui/components/tree-view/_themes.tsx:
- Around line 988-1000: The button currently only stops propagation on click, so
pointerdown still bubbles and can start a row drag; add an onPointerDown handler
to the same button element (the one with type="button" and onClick that calls
e.stopPropagation()) that calls e.stopPropagation() (and optionally
e.preventDefault() if you want to prevent focus/drag-start) to prevent the
pointerdown from bubbling into the row drag logic; update both flag buttons (the
visibility/lock buttons rendered by this component) so their onPointerDown
blocks propagation.

In `@editor/app/`(tools)/(packages)/packages/[%40grida]/tree-view/page.tsx:
- Line 27: The code references the React namespace type React.ComponentType
(used around the component typings at React.ComponentType) but doesn't import
React, which will fail under the modern JSX transform; add an explicit type-only
import such as import type React from "react" (or import React from "react") at
the top of the file so React.ComponentType is resolved, then rerun TypeScript
checks to confirm the type error is fixed.

In `@editor/components/ui-editor/tree.tsx`:
- Around line 137-152: Replace the interactive <span> used for the chevron with
a semantic <button type="button"> element to restore keyboard and assistive-tech
accessibility: keep the data-slot="tree-item-chevron", className, the
onPointerDown handler (e.stopPropagation()), and the existing onClick behavior
that calls onChevronClick, add an appropriate aria-label (e.g.,
aria-label="Toggle folder" or similar) and ensure the ChevronDownIcon remains
the child; update any CSS expectations if necessary but do not change the
onChevronClick function or folder conditional logic.

In
`@editor/grida-canvas-react-starter-kit/starterkit-hierarchy/node-hierarchy-tree-item.tsx`:
- Around line 156-157: The action buttons (lock/visibility) currently only stop
click propagation so the row-level onPointerDown still arms row drag; update the
button handlers to also stop pointerdown propagation by adding an onPointerDown
handler that calls e.stopPropagation() alongside the existing onClick handler on
each action button (the same change should be applied to the other action
buttons in the block around where onClick/onPointerDown are used, e.g., the
second group at lines ~213-231), ensuring the row-level onPointerDown (row
drag/select) does not receive the pointerdown from those buttons.

In `@editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx`:
- Around line 568-607: The drag lifecycle isn’t gated to the originating pointer
so a different pointer can drive onMove/onUp; fix by recording the initiating
pointerId (use pendingRef.current.pointerId or store activePointerId when
starting drag in onMove) and early-return in onMove and onUp unless e.pointerId
(or the eventless up handler should accept a pointerId param or check the stored
active id source) matches that stored id; apply the same pointerId gating logic
for the other handlers referenced around lines 612–620 (same use of pendingRef,
dragRef, controller.startDrag, applyHit, commitDrag) so only the initiating
pointer can move/commit the drag.
- Around line 76-79: The comment above the TREE_INDENT constant contains a
CI-failing typo "mis-resolve"; update that word to a recognized term (for
example "mishandle" or "mis-handle") in the comment near the TREE_INDENT
declaration so the typo check passes while keeping the original meaning; locate
the comment that references `<Tree indent>` and the horizontal pop-out geometry
in tree-node.tsx and replace "mis-resolve" with the chosen correct word.

In `@editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-scene.tsx`:
- Around line 284-299: The pointercancel path must not reuse the pointerup logic
because a cancelled pointer should abort the drag instead of committing it;
change the listeners so pointerup calls the existing onUp (which does
controller.commitDrag(), clears dragRef, pendingRef, and setDrop) but
pointercancel calls a new onCancel handler that only clears dragRef.current,
pendingRef.current, and calls setDrop(null) (or calls a dedicated
controller.cancelDrag() if such exists) without invoking controller.commitDrag;
update the addEventListener/removeEventListener calls to register onCancel for
"pointercancel" and leave "pointermove" and "pointerup" using onMove and onUp
respectively.

In `@editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-source.ts`:
- Around line 48-53: refresh() currently always calls this._snapshot(),
increments this._version and notifies this._listeners even if the node map did
not change; change refresh() to only increment _version and call listeners when
the snapshot actually changed — either make _snapshot() return a boolean/changed
flag or have refresh capture the previous node map (or a stable fingerprint) and
compare it to the new one before bumping this._version and notifying
this._listeners; update any callers accordingly (methods referenced: refresh(),
_snapshot(), _version, _listeners).

In `@packages/grida-tree-view/FEEDBACKS.md`:
- Around line 512-519: The fenced code block shown (starting at the block that
begins with "source: <root>[a,b,c]; a:[a1,a2]   flatten.reverseChildren: true")
is missing a language tag which triggers MD040; add a language identifier (for
example `text` or `ts`) immediately after the opening triple backticks (e.g.,
change ``` to ```text) so the block is recognized correctly by the linter.

---

Duplicate comments:
In `@packages/grida-tree-view/src/drag.ts`:
- Around line 93-101: resolveDropPosition currently returns an "into"
destination for dir === "into" without verifying the target can contain
children; update the branch in resolveDropPosition to call
source.isContainer(over) (or similar API used for container checks) and if it
returns false, reject the "into" drop (e.g., return null or fall back to a
non-"into" placement) instead of returning an invalid into target; adjust the
code around overNode, filtered, and the return object so the into case only
executes when source.isContainer(over) is true.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1f20c789-aef8-4edb-8826-1af7efe18aea

📥 Commits

Reviewing files that changed from the base of the PR and between ecaa5cb and 83022bc.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (21)
  • editor/app/(dev)/ui/comp/comp-569.tsx
  • editor/app/(dev)/ui/components/tree-view/_fixtures.ts
  • editor/app/(dev)/ui/components/tree-view/_showcase.tsx
  • editor/app/(dev)/ui/components/tree-view/_themes.tsx
  • editor/app/(dev)/ui/components/tree/page.tsx
  • editor/app/(dev)/ui/page.tsx
  • editor/app/(tools)/(packages)/packages/[%40grida]/tree-view/page.tsx
  • editor/components/ui-editor/tree.tsx
  • editor/grida-canvas-react-starter-kit/starterkit-hierarchy/node-hierarchy-tree-item.tsx
  • editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx
  • editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-scene.tsx
  • editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-source.ts
  • editor/grida-canvas-react-starter-kit/starterkit-hierarchy/utils.ts
  • editor/package.json
  • packages/grida-tree-view/FEEDBACKS.md
  • packages/grida-tree-view/README.md
  • packages/grida-tree-view/__tests__/constraints.test.ts
  • packages/grida-tree-view/__tests__/controller.test.ts
  • packages/grida-tree-view/__tests__/drag.test.ts
  • packages/grida-tree-view/src/controller.ts
  • packages/grida-tree-view/src/drag.ts
💤 Files with no reviewable changes (3)
  • editor/grida-canvas-react-starter-kit/starterkit-hierarchy/utils.ts
  • editor/app/(dev)/ui/comp/comp-569.tsx
  • editor/app/(dev)/ui/components/tree/page.tsx
✅ Files skipped from review due to trivial changes (1)
  • packages/grida-tree-view/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
  • editor/package.json
  • editor/app/(dev)/ui/page.tsx
  • editor/app/(dev)/ui/components/tree-view/_fixtures.ts
  • packages/grida-tree-view/tests/controller.test.ts
  • packages/grida-tree-view/tests/constraints.test.ts
  • packages/grida-tree-view/src/controller.ts

Comment thread editor/app/(dev)/ui/components/tree-view/_showcase.tsx
Comment on lines +988 to +1000
<button
type="button"
onClick={(e) => {
e.stopPropagation();
onClick();
}}
title={title}
aria-label={title}
className={[
"size-5 inline-flex items-center justify-center rounded transition-opacity",
base,
alwaysVisible ? "opacity-100" : "opacity-0 group-hover/row:opacity-100",
].join(" ")}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Stop pointerdown from bubbling out of the flag buttons.

The row arms drag on pointerdown, but this button only stops propagation on click. A slight pointer movement while toggling visibility or lock can start a drag instead of just pressing the button.

Suggested fix
     <button
       type="button"
+      onPointerDown={(e) => e.stopPropagation()}
       onClick={(e) => {
         e.stopPropagation();
         onClick();
       }}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<button
type="button"
onClick={(e) => {
e.stopPropagation();
onClick();
}}
title={title}
aria-label={title}
className={[
"size-5 inline-flex items-center justify-center rounded transition-opacity",
base,
alwaysVisible ? "opacity-100" : "opacity-0 group-hover/row:opacity-100",
].join(" ")}
<button
type="button"
onPointerDown={(e) => e.stopPropagation()}
onClick={(e) => {
e.stopPropagation();
onClick();
}}
title={title}
aria-label={title}
className={[
"size-5 inline-flex items-center justify-center rounded transition-opacity",
base,
alwaysVisible ? "opacity-100" : "opacity-0 group-hover/row:opacity-100",
].join(" ")}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/app/`(dev)/ui/components/tree-view/_themes.tsx around lines 988 -
1000, The button currently only stops propagation on click, so pointerdown still
bubbles and can start a row drag; add an onPointerDown handler to the same
button element (the one with type="button" and onClick that calls
e.stopPropagation()) that calls e.stopPropagation() (and optionally
e.preventDefault() if you want to prevent focus/drag-start) to prevent the
pointerdown from bubbling into the row drag logic; update both flag buttons (the
visibility/lock buttons rendered by this component) so their onPointerDown
blocks propagation.

Comment on lines +137 to +152
{folder && (
<span
data-slot="tree-item-chevron"
onPointerDown={(e) => e.stopPropagation()}
onClick={
onChevronClick
? (e) => {
e.stopPropagation();
onChevronClick(e);
}
: undefined
}
className="-m-1 flex size-5 items-center justify-center"
>
<ChevronDownIcon className="text-muted-foreground size-3 in-aria-[expanded=false]:-rotate-90" />
</span>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

# First, let's check if the file exists and read the specific section
cd /repo && wc -l editor/components/ui-editor/tree.tsx

Repository: gridaco/grida

Length of output: 113


🏁 Script executed:

# Read the file around the specified lines (137-152) with context
cd /repo && sed -n '120,160p' editor/components/ui-editor/tree.tsx | cat -n

Repository: gridaco/grida

Length of output: 113


🏁 Script executed:

# Also check the entire file structure to understand the component
cd /repo && head -50 editor/components/ui-editor/tree.tsx | cat -n

Repository: gridaco/grida

Length of output: 113


🏁 Script executed:

pwd && git ls-files | grep "editor/components/ui-editor/tree.tsx"

Repository: gridaco/grida

Length of output: 113


🏁 Script executed:

# Read the file around lines 137-152
sed -n '120,165p' /home/jailuser/git/editor/components/ui-editor/tree.tsx | cat -n

Repository: gridaco/grida

Length of output: 2011


🏁 Script executed:

# Also check the component definition and imports
head -40 /home/jailuser/git/editor/components/ui-editor/tree.tsx | cat -n

Repository: gridaco/grida

Length of output: 1425


🏁 Script executed:

# Check the complete TreeItemLabel component and surrounding code
sed -n '1,50p' /home/jailuser/git/editor/components/ui-editor/tree.tsx | cat -n

Repository: gridaco/grida

Length of output: 1683


🏁 Script executed:

# Search for any role or accessibility attributes on tree-item-chevron
rg "tree-item-chevron" /home/jailuser/git/editor/components/ui-editor/tree.tsx -A 10 -B 5

Repository: gridaco/grida

Length of output: 486


🏁 Script executed:

# Check if there are any stylings that might conflict with button element
rg "tree-item-chevron" /home/jailuser/git/ -A 2 -B 2

Repository: gridaco/grida

Length of output: 483


Use a <button> element for the chevron control instead of a <span>.

This interactive control lacks button semantics and keyboard accessibility. Converting it to a <button type="button"> with an appropriate aria-label ensures it can be activated via keyboard and properly announced by assistive technologies.

Suggested fix
-        <span
+        <button
+          type="button"
+          aria-label="Toggle folder"
           data-slot="tree-item-chevron"
           onPointerDown={(e) => e.stopPropagation()}
           onClick={
             onChevronClick
               ? (e) => {
                   e.stopPropagation();
                   onChevronClick(e);
                 }
               : undefined
           }
           className="-m-1 flex size-5 items-center justify-center"
         >
           <ChevronDownIcon className="text-muted-foreground size-3 in-aria-[expanded=false]:-rotate-90" />
-        </span>
+        </button>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{folder && (
<span
data-slot="tree-item-chevron"
onPointerDown={(e) => e.stopPropagation()}
onClick={
onChevronClick
? (e) => {
e.stopPropagation();
onChevronClick(e);
}
: undefined
}
className="-m-1 flex size-5 items-center justify-center"
>
<ChevronDownIcon className="text-muted-foreground size-3 in-aria-[expanded=false]:-rotate-90" />
</span>
{folder && (
<button
type="button"
aria-label="Toggle folder"
data-slot="tree-item-chevron"
onPointerDown={(e) => e.stopPropagation()}
onClick={
onChevronClick
? (e) => {
e.stopPropagation();
onChevronClick(e);
}
: undefined
}
className="-m-1 flex size-5 items-center justify-center"
>
<ChevronDownIcon className="text-muted-foreground size-3 in-aria-[expanded=false]:-rotate-90" />
</button>
)}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/components/ui-editor/tree.tsx` around lines 137 - 152, Replace the
interactive <span> used for the chevron with a semantic <button type="button">
element to restore keyboard and assistive-tech accessibility: keep the
data-slot="tree-item-chevron", className, the onPointerDown handler
(e.stopPropagation()), and the existing onClick behavior that calls
onChevronClick, add an appropriate aria-label (e.g., aria-label="Toggle folder"
or similar) and ensure the ChevronDownIcon remains the child; update any CSS
expectations if necessary but do not change the onChevronClick function or
folder conditional logic.

Comment on lines +156 to +157
onClick={onClick}
onPointerDown={onPointerDown}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Prevent action-button pointerdown from bubbling into row drag/select.

With row-level onPointerDown on Line 157, pressing lock/visibility buttons can still arm row drag because only click propagation is stopped. Stop pointerdown propagation on these buttons too.

Suggested fix
           <NodeActionButton
             alwaysVisible={node.locked}
+            onPointerDown={(e) => e.stopPropagation()}
             onClick={(e) => {
               e.stopPropagation();
               onToggleLocked();
             }}
           >
@@
           <NodeActionButton
             alwaysVisible={!node.active}
+            onPointerDown={(e) => e.stopPropagation()}
             onClick={(e) => {
               e.stopPropagation();
               onToggleActive();
             }}
           >

Also applies to: 213-231

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@editor/grida-canvas-react-starter-kit/starterkit-hierarchy/node-hierarchy-tree-item.tsx`
around lines 156 - 157, The action buttons (lock/visibility) currently only stop
click propagation so the row-level onPointerDown still arms row drag; update the
button handlers to also stop pointerdown propagation by adding an onPointerDown
handler that calls e.stopPropagation() alongside the existing onClick handler on
each action button (the same change should be applied to the other action
buttons in the block around where onClick/onPointerDown are used, e.g., the
second group at lines ~213-231), ensuring the row-level onPointerDown (row
drag/select) does not receive the pointerdown from those buttons.

Comment on lines +76 to +79
// Visual indent step (px / depth level). Shared by `<Tree indent>` and the
// horizontal pop-out geometry so the gesture maps 1:1 to what's rendered —
// a mismatch would silently mis-resolve the plain (no-x) reorder.
const TREE_INDENT = 6;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fix CI-blocking typo in comment text.

Line 78 uses mis-resolve, which is failing the typo check. Replace with a recognized word (e.g., mishandle).

🧰 Tools
🪛 GitHub Actions: typos / 0_typos.txt

[error] 78-78: Spelling check failed: mis should be miss, mist.

🪛 GitHub Actions: typos / typos

[error] 78-78: Spelling check: mis should be miss, mist.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx`
around lines 76 - 79, The comment above the TREE_INDENT constant contains a
CI-failing typo "mis-resolve"; update that word to a recognized term (for
example "mishandle" or "mis-handle") in the comment near the TREE_INDENT
declaration so the typo check passes while keeping the original meaning; locate
the comment that references `<Tree indent>` and the horizontal pop-out geometry
in tree-node.tsx and replace "mis-resolve" with the chosen correct word.

Comment on lines +568 to +607
const onMove = (e: PointerEvent) => {
const pending = pendingRef.current;
if (pending && pending.pointerId === e.pointerId && !dragRef.current) {
if (
!passedDragThreshold(
pending.x,
pending.y,
e.clientX,
e.clientY,
THRESHOLD_PX
)
)
return;
// Figma rule: drag the whole selection if the grabbed row is part
// of it, else just the grabbed row.
const items = selectionSet.has(pending.id)
? [...selection]
: [pending.id];
dragRef.current = controller.startDrag(items);
}
const handle = dragRef.current;
if (!handle) return;
lastPointerRef.current = { x: e.clientX, y: e.clientY };
applyHit(handle, e.clientX, e.clientY);
// Kick the auto-scroll loop when the cursor is in the scroller's
// edge zone (and it isn't already ticking).
const scroller = getScrollParent(containerRef.current);
if (scroller && autoScrollRafRef.current === null) {
const rect = scroller.getBoundingClientRect();
const EDGE = 32;
if (e.clientY < rect.top + EDGE || e.clientY > rect.bottom - EDGE)
autoScrollRafRef.current = requestAnimationFrame(tickAutoScroll);
}
};
const onUp = () => {
stopAutoScroll();
if (dragRef.current) {
controller.commitDrag();
dragRef.current = null;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Bind drag lifecycle to the originating pointer id.

After drag starts, onMove/onUp are not constrained to the initiating pointer, so another pointer can move/commit the drag. Gate both handlers by an active pointer id.

Suggested fix
   const autoScrollRafRef = useRef<number | null>(null);
+  const activePointerIdRef = useRef<number | null>(null);
@@
-    const onMove = (e: PointerEvent) => {
+    const onMove = (e: PointerEvent) => {
+      const activePointerId = activePointerIdRef.current;
+      if (activePointerId !== null && e.pointerId !== activePointerId) return;
+
       const pending = pendingRef.current;
       if (pending && pending.pointerId === e.pointerId && !dragRef.current) {
@@
         const items = selectionSet.has(pending.id)
           ? [...selection]
           : [pending.id];
         dragRef.current = controller.startDrag(items);
+        activePointerIdRef.current = pending.pointerId;
       }
@@
-    const onUp = () => {
+    const onUp = (e: PointerEvent) => {
+      const activePointerId = activePointerIdRef.current;
+      if (activePointerId !== null && e.pointerId !== activePointerId) return;
+
       stopAutoScroll();
       if (dragRef.current) {
         controller.commitDrag();
         dragRef.current = null;
       }
+      activePointerIdRef.current = null;
       pendingRef.current = null;
       lastPointerRef.current = null;
       setDrop(null);
     };

Also applies to: 612-620

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx`
around lines 568 - 607, The drag lifecycle isn’t gated to the originating
pointer so a different pointer can drive onMove/onUp; fix by recording the
initiating pointerId (use pendingRef.current.pointerId or store activePointerId
when starting drag in onMove) and early-return in onMove and onUp unless
e.pointerId (or the eventless up handler should accept a pointerId param or
check the stored active id source) matches that stored id; apply the same
pointerId gating logic for the other handlers referenced around lines 612–620
(same use of pendingRef, dragRef, controller.startDrag, applyHit, commitDrag) so
only the initiating pointer can move/commit the drag.

Comment on lines +284 to +299
const onUp = () => {
if (dragRef.current) {
controller.commitDrag();
dragRef.current = null;
}
pendingRef.current = null;
setDrop(null);
};
window.addEventListener("pointermove", onMove);
window.addEventListener("pointerup", onUp);
window.addEventListener("pointercancel", onUp);
return () => {
window.removeEventListener("pointermove", onMove);
window.removeEventListener("pointerup", onUp);
window.removeEventListener("pointercancel", onUp);
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't commit on pointercancel.

A cancelled pointer gesture is an abort, not a drop. Reusing the pointerup path here can emit a reorder for a drag the user never actually completed.

Suggested fix
-    const onUp = () => {
+    const finishDrag = (commit: boolean) => {
       if (dragRef.current) {
-        controller.commitDrag();
+        if (commit) {
+          controller.commitDrag();
+        } else {
+          dragRef.current.cancel();
+        }
         dragRef.current = null;
       }
       pendingRef.current = null;
       setDrop(null);
     };
     window.addEventListener("pointermove", onMove);
-    window.addEventListener("pointerup", onUp);
-    window.addEventListener("pointercancel", onUp);
+    window.addEventListener("pointerup", () => finishDrag(true));
+    window.addEventListener("pointercancel", () => finishDrag(false));
     return () => {
       window.removeEventListener("pointermove", onMove);
-      window.removeEventListener("pointerup", onUp);
-      window.removeEventListener("pointercancel", onUp);
+      window.removeEventListener("pointerup", () => finishDrag(true));
+      window.removeEventListener("pointercancel", () => finishDrag(false));
     };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const onUp = () => {
if (dragRef.current) {
controller.commitDrag();
dragRef.current = null;
}
pendingRef.current = null;
setDrop(null);
};
window.addEventListener("pointermove", onMove);
window.addEventListener("pointerup", onUp);
window.addEventListener("pointercancel", onUp);
return () => {
window.removeEventListener("pointermove", onMove);
window.removeEventListener("pointerup", onUp);
window.removeEventListener("pointercancel", onUp);
};
const finishDrag = (commit: boolean) => {
if (dragRef.current) {
if (commit) {
controller.commitDrag();
} else {
dragRef.current.cancel?.();
}
dragRef.current = null;
}
pendingRef.current = null;
setDrop(null);
};
const onUp = () => finishDrag(true);
const onCancel = () => finishDrag(false);
window.addEventListener("pointermove", onMove);
window.addEventListener("pointerup", onUp);
window.addEventListener("pointercancel", onCancel);
return () => {
window.removeEventListener("pointermove", onMove);
window.removeEventListener("pointerup", onUp);
window.removeEventListener("pointercancel", onCancel);
};
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-scene.tsx`
around lines 284 - 299, The pointercancel path must not reuse the pointerup
logic because a cancelled pointer should abort the drag instead of committing
it; change the listeners so pointerup calls the existing onUp (which does
controller.commitDrag(), clears dragRef, pendingRef, and setDrop) but
pointercancel calls a new onCancel handler that only clears dragRef.current,
pendingRef.current, and calls setDrop(null) (or calls a dedicated
controller.cancelDrag() if such exists) without invoking controller.commitDrag;
update the addEventListener/removeEventListener calls to register onCancel for
"pointercancel" and leave "pointermove" and "pointerup" using onMove and onUp
respectively.

Comment on lines +48 to +53
/** Re-read the editor and bump the version if anything observable moved. */
refresh(): void {
this._snapshot();
this._version++;
for (const l of this._listeners) l();
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't bump the source version on every refresh.

refresh() always increments _version and notifies, even when _snapshot() rebuilt the exact same node map. Because the editor wires this to its document subscription, unrelated document updates will still invalidate every tree selector and rerender the panel.

Suggested fix
   /** Re-read the editor and bump the version if anything observable moved. */
   refresh(): void {
-    this._snapshot();
+    if (!this._snapshot()) return;
     this._version++;
     for (const l of this._listeners) l();
   }
 
-  private _snapshot(): void {
+  private _snapshot(): boolean {
     const { root, getChildIds, getMeta } = this._config;
     const next = new Map<NodeId, TreeNode<T>>();
@@
     };
     visit(root, null);
+    const changed =
+      next.size !== this._nodes.size ||
+      [...next].some(([id, node]) => this._nodes.get(id) !== node);
     this._nodes = next;
+    return changed;
   }

Also applies to: 55-76

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-source.ts`
around lines 48 - 53, refresh() currently always calls this._snapshot(),
increments this._version and notifies this._listeners even if the node map did
not change; change refresh() to only increment _version and call listeners when
the snapshot actually changed — either make _snapshot() return a boolean/changed
flag or have refresh capture the previous node map (or a stable fingerprint) and
compare it to the new one before bumping this._version and notifying
this._listeners; update any callers accordingly (methods referenced: refresh(),
_snapshot(), _version, _listeners).

Comment on lines +512 to +519
```
source: <root>[a,b,c]; a:[a1,a2] flatten.reverseChildren: true
startDrag(["c"])
over("a1","before") -> {a, idx 1, after} // un-flip, no depth
over("a2","before",{desiredDepth: 1}) -> {a, , after} // depth==own: no pop-out (no regression)
over("a2","before",{desiredDepth: 0}) -> {<root>, idx 1, after} // pop-out (post-removal idx)
commit -> move {to:{<root>,1,after,over:a2}}
```
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a language tag to the fenced code block.

The block starting at Line 512 should specify a language (e.g., text or ts) to satisfy MD040.

Suggested fix
-```
+```text
 source: <root>[a,b,c]; a:[a1,a2]   flatten.reverseChildren: true
 startDrag(["c"])
  over("a1","before")                       -> {a,      idx 1, after}  // un-flip, no depth
@@
  commit                                     -> move {to:{<root>,1,after,over:a2}}
</details>

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>

[warning] 512-512: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</details>

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @packages/grida-tree-view/FEEDBACKS.md around lines 512 - 519, The fenced
code block shown (starting at the block that begins with "source: [a,b,c];
a:[a1,a2] flatten.reverseChildren: true") is missing a language tag which
triggers MD040; add a language identifier (for example text or ts)
immediately after the opening triple backticks (e.g., change totext) so
the block is recognized correctly by the linter.


</details>

<!-- fingerprinting:phantom:poseidon:hawk -->

<!-- This is an auto-generated comment by CodeRabbit -->

- Redesign showcases: 16:9 nested-radius card, macOS icons, modern UI fixture
- Add Notion showcase (workspace sidebar + document mock) to dev gallery and marketing
- Merge marketing hero with intro; add quick-start CodeBlock, features grid, CTA
- SEO: full metadata, JSON-LD SoftwareSourceCode, 46 keywords (shadcn/tailwind/headless/react)
- Add reusable macOS desktop primitives (MacOSDesktop/MenuBar/Dock/Window)
- Perf: memo Shape/Chrome/Outline, byId map lookups, single useTreeSnapshot for NotionDocument
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (3)
editor/app/(dev)/ui/components/tree-view/_showcase.tsx (1)

468-471: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Focus the stage when picking a shape so Delete/Backspace works immediately.

onKeyDown is bound to the stage container, but clicking a shape doesn’t programmatically focus that container first.

Proposed fix
 function SvgStage({
   controller,
   artboardId,
   tone,
   accent,
 }: {
@@
 }) {
+  const stageRef = React.useRef<HTMLDivElement | null>(null);
@@
   const onPick = (id: NodeId, e: React.PointerEvent) => {
+    stageRef.current?.focus();
     controller.focus(id);
     controller.select([id], modeFromEvent(e));
   };
@@
     <div
+      ref={stageRef}
       tabIndex={0}
       onKeyDown={(e) => {

Also applies to: 486-493

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/app/`(dev)/ui/components/tree-view/_showcase.tsx around lines 468 -
471, The onPick handler focuses the node but not the stage container, so
keyboard handlers bound to the stage (onKeyDown) don’t receive focus after a
click; update onPick to programmatically focus the stage container before
selecting the node (e.g. call your stage ref’s focus method or a
controller.focusStage() helper) and then call controller.focus(id) and
controller.select([id], modeFromEvent(e)) so Delete/Backspace works immediately;
adjust the same change for the other onPick occurrence (lines 486-493).
editor/app/(dev)/ui/components/tree-view/_fixtures.ts (1)

1108-1149: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard invalid depth/groups before building chain nodes.

At Line 1128 and Line 1148, non-null assertions rely on a non-empty chainIds, but depth <= 0 makes it empty and yields invalid IDs.

Proposed fix
 export function buildDeepFixture(opts: { groups: number; depth: number }): {
   source: InMemoryTreeSource<{ label: string }>;
   expanded: NodeId[];
   maxDepth: number;
   total: number;
 } {
   const { groups, depth } = opts;
+  if (groups <= 0 || depth <= 0) {
+    throw new Error("buildDeepFixture: `groups` and `depth` must be > 0");
+  }
   const nodes: TreeNode<{ label: string }>[] = [];
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/app/`(dev)/ui/components/tree-view/_fixtures.ts around lines 1108 -
1149, Validate and guard against non-positive depth or non-positive groups
before building chain nodes: in the code that reads opts (groups, depth) and
constructs chainIds, add an early check (e.g., if (groups <= 0) return or
normalize groups; if (depth <= 0) handle as depth = 1 or build group nodes with
no chain) so you never rely on chainIds[0] or chainIds[chainIds.length - 1] with
non-null assertions; update the logic around variables chainIds, nodes.push(...)
that reference chainIds[0] and chainIds[chainIds.length - 1] (and the expanded
array population) to either skip chain construction when depth <= 0 or to
produce a single leaf id safely without using the ! operator.
editor/app/(dev)/ui/components/tree-view/_themes.tsx (1)

998-1003: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Stop pointerdown propagation in RowFlagButton to prevent accidental drag starts.

Row drag starts on pointer down, so stopping only click is too late for these embedded controls.

Proposed fix
   return (
     <button
       type="button"
+      onPointerDown={(e) => e.stopPropagation()}
       onClick={(e) => {
         e.stopPropagation();
         onClick();
       }}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/app/`(dev)/ui/components/tree-view/_themes.tsx around lines 998 -
1003, In the RowFlagButton component the button only stops propagation on click,
which is too late for drag-starts; add an onPointerDown handler to the same
<button> (e.g., onPointerDown={(e) => e.stopPropagation()}) so pointerdown
events are stopped immediately and prevent accidental row drags—keep the
existing onClick handler intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@editor/app/`(dev)/ui/components/tree-view/_fixtures.ts:
- Around line 1108-1149: Validate and guard against non-positive depth or
non-positive groups before building chain nodes: in the code that reads opts
(groups, depth) and constructs chainIds, add an early check (e.g., if (groups <=
0) return or normalize groups; if (depth <= 0) handle as depth = 1 or build
group nodes with no chain) so you never rely on chainIds[0] or
chainIds[chainIds.length - 1] with non-null assertions; update the logic around
variables chainIds, nodes.push(...) that reference chainIds[0] and
chainIds[chainIds.length - 1] (and the expanded array population) to either skip
chain construction when depth <= 0 or to produce a single leaf id safely without
using the ! operator.

In `@editor/app/`(dev)/ui/components/tree-view/_showcase.tsx:
- Around line 468-471: The onPick handler focuses the node but not the stage
container, so keyboard handlers bound to the stage (onKeyDown) don’t receive
focus after a click; update onPick to programmatically focus the stage container
before selecting the node (e.g. call your stage ref’s focus method or a
controller.focusStage() helper) and then call controller.focus(id) and
controller.select([id], modeFromEvent(e)) so Delete/Backspace works immediately;
adjust the same change for the other onPick occurrence (lines 486-493).

In `@editor/app/`(dev)/ui/components/tree-view/_themes.tsx:
- Around line 998-1003: In the RowFlagButton component the button only stops
propagation on click, which is too late for drag-starts; add an onPointerDown
handler to the same <button> (e.g., onPointerDown={(e) => e.stopPropagation()})
so pointerdown events are stopped immediately and prevent accidental row
drags—keep the existing onClick handler intact.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4e08e579-01ec-46d2-a60a-6127cc789ea5

📥 Commits

Reviewing files that changed from the base of the PR and between 83022bc and 7ffb644.

📒 Files selected for processing (26)
  • editor/app/(dev)/ui/components/tree-view/_fixtures.ts
  • editor/app/(dev)/ui/components/tree-view/_showcase.tsx
  • editor/app/(dev)/ui/components/tree-view/_themes.tsx
  • editor/app/(dev)/ui/components/tree-view/page.tsx
  • editor/app/(tools)/(packages)/packages/[%40grida]/tree-view/layout.tsx
  • editor/app/(tools)/(packages)/packages/[%40grida]/tree-view/page.tsx
  • editor/components/frames/macos-desktop.tsx
  • editor/public/assets/macos/icons/blender.webp
  • editor/public/assets/macos/icons/figma.webp
  • editor/public/assets/macos/icons/finder.webp
  • editor/public/assets/macos/icons/freeform.webp
  • editor/public/assets/macos/icons/grida.webp
  • editor/public/assets/macos/icons/logic-pro.webp
  • editor/public/assets/macos/icons/maps.webp
  • editor/public/assets/macos/icons/messages.webp
  • editor/public/assets/macos/icons/music.webp
  • editor/public/assets/macos/icons/notes.webp
  • editor/public/assets/macos/icons/notion.webp
  • editor/public/assets/macos/icons/reminders.webp
  • editor/public/assets/macos/icons/safari.webp
  • editor/public/assets/macos/icons/trash-empty.webp
  • editor/public/assets/macos/icons/trash-full.webp
  • editor/public/assets/macos/icons/vscode.webp
  • editor/public/assets/macos/icons/xcode.webp
  • editor/public/assets/macos/wallpapers/tahoe-beach.webp
  • editor/resources.ts
✅ Files skipped from review due to trivial changes (1)
  • editor/resources.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • editor/app/(dev)/ui/components/tree-view/page.tsx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant